Instrument bundler version #3283
Conversation
jurre
force-pushed
the
jurre/instrument-package-manager-versions
branch
from
e6b2063
to
5038e89
Compare
|
|
||
| require "active_support/notifications" | ||
|
|
||
| module Dependabot |
There was a problem hiding this comment.
Thoughts on keeping track of event names here as constants?
There was a problem hiding this comment.
Hmm, it does make it harder to remove the instrumentation. When this is just a string, we can keep the subscribe call in the calling code while we remove all of the instrumentation code, the calling code will just noop. Now we need to be careful to not remove the constant because it would break upstream usage
There was a problem hiding this comment.
In this single scenario I don't think ^ matters much, but when thinking about a generic system that lets us instrument all sort of things, I feel like optimizing to make it easy to add/delete notifications without friction is something to strive for. Thoughts?
There was a problem hiding this comment.
Good point, maybe doesn't make sense to add all notifications as constants but seems ok for ones that we care more about routing to specific places, for this one we'll probably want to end up persisting on update jobs directly whereas other ones we might just want to pass straight through to datadog?
There was a problem hiding this comment.
I agree that ideally it would be easier to add more, but I'm very mildly paranoid that core, as a public library, could have instrumentation added to it that we might not want to relay through our runner to instrumentation services.
Having a list of constants in Dependabot Core allows us to programmatically filter events as we can, by default relay all constants in Dependabot::Notifications.
We'd have to consciously bump dependabot-core in our runner to allow new notification to relay which would decouple a core upgrade from a sudden surge of new events hitting our instrumentation layer.
</tinfoil-hat>
There was a problem hiding this comment.
Good point @brrygrdn, my thinking was that we would only subscribe to specific events and not relay anything, but I can definitely see us moving towards that approach
jurre
force-pushed
the
jurre/instrument-package-manager-versions
branch
from
5038e89
to
eea5ff7
Compare
|
|
||
| # TODO: Replace with bundler_version once it is implemented | ||
| def self.actual_bundler_version(lockfile) | ||
| return V1 unless lockfile |
There was a problem hiding this comment.
Should we return unknown here?
I guess the question ultimately is: should we instrument the version used or the version specified
There was a problem hiding this comment.
Ah yes, I think it would be useful to gauge the usage of current users so returning V1 makes sense to me 👍
There was a problem hiding this comment.
Hm I just re-read this, good point when there's no lockfile, I guess we'll eventually default to V2 here so maybe a good idea to return uknown actually so we can get a sense for how many projects would be impacted by flipping the default, thoughts?
There was a problem hiding this comment.
Yeah, agreed 👍 we'll probably want to keep this separate helper so I'll think of a more permanent name
jurre
force-pushed
the
jurre/instrument-package-manager-versions
branch
from
eea5ff7
to
51a0fc3
Compare
jurre
changed the title
jurre
force-pushed
the
jurre/instrument-package-manager-versions
branch
from
51a0fc3
to
c3071cf
Compare
Instrument the version of bundler that a package has specified.
This way we can collect metrics about usage of these gems.
This PR implements just a single ecosystem, but we've tried to take into account changes that might be required for other ecosystems. For python, for example, it is possible to have multiple package managers configured, so we instrument a hash of package manager => version.
Planning to have the consumer side of this set up and reporting actual data before adding more ecosystems to the mix.